Skip to content

fix: use structured YAML parsing for frontmatter and scripts in process_template - #4552

Open
darion-yaphet wants to merge 1 commit into
github:mainfrom
darion-yaphet:fix/structured-yaml-parsing
Open

darion-yaphet wants to merge 1 commit into
github:mainfrom
darion-yaphet:fix/structured-yaml-parsing

Conversation

@darion-yaphet

@darion-yaphet darion-yaphet commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Description

In `IntegrationBase.process_template`, extracting and stripping the `scripts:` block from command templates previously relied on a hand-rolled line-by-line

scanning loop and regex matching (script_pattern = re.compile(r"^\s*([A-Za-z0-9_-]+):\s*(.+)$")).

This approach had multiple issues:
- It failed on valid YAML syntax in frontmatter, such as comments between script entries, quoted script paths, or folded/literal multi-line block scalars (`>` or

|).
- It risked accidental matches if scripts: was referenced inside Markdown body text or code examples.
- Stripping scripts: relied on line indentation checks rather than structured object manipulation.

This PR replaces the hand-rolled text traversal with structured YAML handling using `yaml.safe_load` and `yaml.dump`:
1. Safely parses frontmatter into a Python dictionary to extract `scripts`.
2. Replaces `{SCRIPT}` in the template body.
3. Removes `scripts` (`frontmatter_dict.pop("scripts", None)`) and dumps the cleaned frontmatter back via `yaml.dump(..., sort_keys=False, allow_unicode=True,

width=float("inf")). 4. Adds unit tests in tests/integrations/test_base.py` covering YAML comments, quoted script commands, and folded scalar descriptions.

Testing

- [x] Tested locally with `uv run specify --help`
- [x] Ran existing tests with `uv sync && uv run pytest`
  - `pytest tests/integrations/test_base.py tests/test_command_template_py_scripts.py` (118 tests passed)
  - `ruff check src/specify_cli/integrations/base.py tests/integrations/test_base.py` (All checks passed)
- [ ] Tested with a sample project (if applicable)

## AI Disclosure

- [ ] I **did not** use AI assistance for this contribution
- [x] I **did** use AI assistance (describe below)

Assisted by Google Antigravity / Gemini for refactoring `process_template` to use `yaml.safe_load` and `yaml.dump`, adding test coverage in

tests/integrations/test_base.py, and drafting commit/PR descriptions.

…ss_template

Replace hand-rolled line-by-line scanning and regex matching in IntegrationBase.process_template with yaml.safe_load and yaml.dump. Properly support YAML comments, folded block scalars, and quoted values in command templates while stripping the scripts key cleanly. Add unit tests in tests/integrations/test_base.py to verify structured YAML frontmatter parsing.
@mnriem mnriem added the triage-can-wait Verdict: valid and in-scope but deprioritized; held behind the evidence gate label Sep 12, 2026
@mnriem
mnriem requested a balanced review from Copilot September 15, 2026 15:37
@mnriem

mnriem commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Supporting quoted and folded YAML script commands is worthwhile, but this implementation needs corrections before merge. Please require an exact opening frontmatter delimiter: ---- currently causes ordinary Markdown content to be consumed and discarded. Also report YAML parsing failures explicitly rather than emitting empty frontmatter with unresolved {SCRIPT}.

Both added tests currently pass against the pre-change implementation. Please assert the actual rendered invocation for quoted and folded script commands, with tests that fail before the fix and pass afterward, and cover the content-preservation/error cases above. The current CI runs also await maintainer approval.

Drafted for @mnriem by GitHub Copilot (model: GPT-6 Astra).

@mnriem mnriem added author-needs-tests Real change but missing a regression test — add one that fails before / passes after author-awaiting Waiting on author response triage-nice-to-have Verdict: evidence-backed fix or greenlit feature — land after review and removed triage-can-wait Verdict: valid and in-scope but deprioritized; held behind the evidence gate labels Sep 15, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Frontmatter can be misidentified or erased, and the new tests do not reliably reproduce the reported regressions.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Replaces manual frontmatter scanning with structured YAML parsing and serialization.

Changes:

  • Parses and removes scripts through PyYAML.
  • Adds coverage for quoted commands and folded descriptions.
File summaries
File Description
src/specify_cli/integrations/base.py Implements structured frontmatter processing.
tests/integrations/test_base.py Adds YAML processing tests.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 4
  • Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +835 to +836
if content.startswith("---"):
lines = content.splitlines(keepends=True)
Comment on lines +849 to +859
try:
parsed = yaml.safe_load(fm_text)
if isinstance(parsed, dict):
frontmatter_dict = parsed
raw_scripts = frontmatter_dict.get("scripts")
if isinstance(raw_scripts, dict):
for k, v in raw_scripts.items():
if isinstance(v, str):
script_commands[str(k)] = v.strip()
except yaml.YAMLError:
pass
Comment on lines +783 to +787
result = IntegrationBase.process_template(content, "agent", "sh")
assert ".specify/scripts/bash/check-prerequisites.sh --json" in result
assert "scripts:" not in result
assert "description:" in result
assert "Test with quotes and comments" in result
Comment on lines +789 to +803
def test_folded_yaml_scalar_preserved(self):
content = (
"---\n"
"description: >\n"
" A multi-line folded description\n"
" that spans multiple lines.\n"
"scripts:\n"
" sh: scripts/bash/check-prerequisites.sh\n"
"---\n"
"Run {SCRIPT}."
)
result = IntegrationBase.process_template(content, "agent", "sh")
assert ".specify/scripts/bash/check-prerequisites.sh" in result
assert "scripts:" not in result
assert "A multi-line folded description" in result
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author-awaiting Waiting on author response author-needs-tests Real change but missing a regression test — add one that fails before / passes after triage-nice-to-have Verdict: evidence-backed fix or greenlit feature — land after review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants